-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(cdk-experimental/toolbar): toolbar and widget #31610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
selector: '[cdkToolbarWidget]', | ||
exportAs: 'cdkToolbarWidget', | ||
host: { | ||
'role': 'button', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
widgets might not always be buttons, we should leave the role assignment to developers. https://www.w3.org/WAI/ARIA/apg/patterns/toolbar/examples/toolbar/ at least there are links and spinbuttons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the 'role' attribute causes some issues with the _getItem
function. This function is called when a pointer event occurs on a widget, and the current solution involves searching for the closest target with role button or role radio. By removing this role keyboard navigation is not updated in position when a widget is clicked. The _getItem
function is likely to change in the future- do you have a recommended way of altering it now to prevent this issue while removing the role?
/** Finds the Toolbar Widget associated with a pointer event target. */
private _getItem(e: PointerEvent): RadioButtonPattern<V> | ToolbarWidgetPattern | undefined {
if (!(e.target instanceof HTMLElement)) {
return undefined;
}
// Assumes the target or its ancestor has role="radio" or role="button"
const element = e.target.closest('[role="button"], [role="radio"]');
return this.inputs.items().find(i => i.element() === element);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a larger discussion but we might want to lean on data-
attributes to resolve this
...this, | ||
id: this.id, | ||
element: this.element, | ||
disabled: computed(() => this._cdkToolbar.disabled() || this.disabled() || false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list behavior handles the parent/child disabled logic internally, so passing only the current disabled state for the widget should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the fact that widgets do not have selection logic directly applied, the list behavior isn't fully in charge of whether or not a widget can be interacted with. Without having the widget disabled state mimic that of the parent toolbar the native buttons can still be interacted with when the toolbar is disabled. Since interaction begins at the inner most level the Toolbar cannot prevent the click event from propagating to the button. The extra || false
is not needed though. I think it's worth discussing having a cleaner way of preventing interaction within a toolbar though.
} else { | ||
/** Item is a Toolbar Widget, manually select it */ | ||
if (activeItem && activeItem.element() && !activeItem.disabled()) | ||
activeItem.element().click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's investigate whether we should allow event manager to propagate some events. I imagine there are also cases like developers adding a wrapper on top of a button widget to trigger side-effects (e.g. tracking) or something, and this solution will only trigger the target button.
}; | ||
|
||
/** Represents the required inputs for a toolbar widget in a toolbar. */ | ||
export interface ToolbarWidgetInputs extends Omit<ListItem<any>, 'searchTerm' | 'value' | 'index'> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use generics instead of any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is presently set as any
due to the fact this typing is solely required for selection and as such is never set or used in the widget. Should it still be changed to a generic despite this?
/** Represents an item in the list. */
export type ListItem<V> = ListTypeaheadItem &
ListNavigationItem &
ListSelectionItem<V> &
ListFocusItem;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there's some way to clean up the List behavior to make this part of the authoring experience nicer. It's very common for UI Patterns to rely on 3 of the 4 sub-behaviors, and each time it is annoying to need to get the type definitions right
We might benefit from authoring more precise types within the List that UI Patterns could pick from:
export type ListInputs<T extends ListItem<V>, V> = ListFocusInputs<T> &
ListNavigationInputs<T> &
ListSelectionInputs<T, V> &
ListTypeaheadInputs<T>;
export type ListInputsWithoutTypeahead<T extends ListItem<V>, V> = Omit<
ListInputs<T, V>,
keyof ListTypeaheadInputs<T>
>;
export type ListInputsWithoutSelection<T extends ListItem<V>, V> = Omit<
ListInputs<T, V>,
keyof ListSelectionInputs<T, V>
>;
export type ListInputsWithoutSelectionOrTypeahead<T extends ListItem<V>, V> = Omit<
ListInputs<T, V>,
keyof ListSelectionInputs<T, V> | keyof ListTypeaheadInputs<T>
>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be a good way forward- especially considering how many patterns only need a subset of what List provides.
Deployed dev-app for 664a27b to: https://ng-dev-previews-comp--pr-angular-components-31610-dev-t4erc2pi.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
…lar#31665) Fixes that the horizontal stepper was leaving a blank space if the step's label is empty. Fixes angular#31655.
Draft PR for first revision of CdkToolbar and CdkToolbarWidget.